Skip to content
This repository was archived by the owner on Oct 3, 2024. It is now read-only.

Add a few features #196

Closed
wants to merge 3 commits into from
Closed

Add a few features #196

wants to merge 3 commits into from

Conversation

mgolebiowski95
Copy link

@mgolebiowski95 mgolebiowski95 commented Jan 12, 2018

  • prevent rationale dialog from showing twice
  • app settings dialog created based on dialog fragment
  • now you have possibility to check app setting dialog is showing
  • sample how prevent call EasyPermission.requestPermission(...) when android permission dialog is showing (too works after rotation changed)
  • including kotlin

I sorry for using kotlin :-)

- app settings dialog created base on dialog fragment
- now you have possibility to check app setting dialog is showing
- sample how prevent call EasyPermission.requestPermission(...) when android permission dialog is showing (too works after rotation changed)
- including kotlin
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@mgolebiowski95
Copy link
Author

mgolebiowski95 commented Jan 12, 2018 via email

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@samtstern
Copy link
Contributor

@mgolebiowski95 thanks for sending this PR! Looks like there's a lot going on here:

  1. Adding a sample in Kotlin, moving the Java sample
  2. Creating the AbstractDialogFragment class and using it for app settings dialogs
  3. Adding a new callback for when permissions are requested
  4. Prevent double-showing of the dialog (this one I am already doing in Prevent rationale dialog from showing twice #195)

Can you explain your rationale for #2 and #3? Also if we go forward with these we should probably split this into multiple pull requests as there are many unrelated things going on here.

@SUPERCILEX
Copy link
Contributor

First off, thanks for the PR! For future reference, I'd recommend opening an issue first so we can discuss what features to add. Unfortunately, I see a few problems with this PR:

  • Kotlin
    • I don't think we can include Kotlin in the library module yet because not everyone will be ready for a 1 MB increase in library size
    • As for the sample, not everyone will know kotlin so we still need to support the Java versions
  • The bug fix to not show the dialog when it is already showing has been done by @samtstern: Prevent rationale dialog from showing twice #195

Ask for your feature, could you show an example use case for how you would use it?

Thanks for your work!

@SUPERCILEX
Copy link
Contributor

@samtstern oh lol, you beat me to it. 😉

@mgolebiowski95
Copy link
Author

@SUPERCILEX yes, I shoud write this in issue section, sorry for that

@samtstern I know that you done fix #195, I append this to my code because I want test something before you have pushed the changes.

I only want dissolve one more problem with showing RationaleDialog under AppSettingDialog.

In my uncommon case when I rotate phone / go to background -> come back to the app.
like #193 clear application data -> grand first permission, other deny -> rotate phone a few times / go to background and come back to the app (RationaleDialog show under AppSettingDialogdialog)

Now in library, AppSettingDialog isn't based on DialogFragment and I couldn't check is showing like RationaleDialog via 'getFragmentManager().findFragmentByTag(RationaleDialog .TAG) != null'

Other idea is, if library invoke method which shows AndroidPermissinoDialog (e.g. Activity.requestPermission(...)) callback move outside from library. It would be good to know if library will show AndroidPermissionDialog. Then would be possibility to save this state and wait for doing something else. I my case I want to know if the AndroidPermissionDialog is is shown.

I presented the idea how You can improve your library. Now I know that it would be better to append this issue to issue section.

@samtstern
Copy link
Contributor

@mgolebiowski95 thanks for sharing your ideas! I agree, let's discuss each of these individual things in an Issue and see if we can come up with a solution together.

I am going to close this PR so we can go with that plan. We can always re-open it if we need to.

@samtstern samtstern closed this Jan 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants